Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New features for pathway identification and charge barrier analysis #149

Merged
merged 21 commits into from
Jan 26, 2021

Conversation

jmmshn
Copy link
Contributor

@jmmshn jmmshn commented Jan 6, 2021

New features for pathway identification and charge barrier analysis

  • Added method for performing Dijkstra-like pathway analysis on periodic graph.
  • Added the functionality to find a sequence of hops that take you from site-i to periodic image of site-i thus and identify intercalating pathways
  • Added functionality to assign a cost to individual migration events either using the integrated charge density along a tube or along s series of image sites determined by the PathFinder tool from pymatgen.

@jmmshn jmmshn changed the title [WIP] New features for pathway identification and charge barrier analysis New features for pathway identification and charge barrier analysis Jan 12, 2021
@jmmshn
Copy link
Contributor Author

jmmshn commented Jan 15, 2021

Hi @shyuep, apologies for how this large this PR is. Much of this we kept in a private repo over last year for internal projects. Will it be possible to give me write access?

@@ -0,0 +1,14 @@
repos:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unclear what this file is for or why it needs to be committed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is to help format the changed files prior to committing
Committing this ensures that the code changes will pass most linting checks after a push.
We use this for pymatgen:
https://github.com/materialsproject/pymatgen/blob/master/.pre-commit-config.yaml

If you do not install the hook it does nothing but if it's installed it's just a nice little creature comfort thing for developers.

symprec=0.1,
vac_mode=False,
name: str = None,
):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some functional annotations would be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

# if d["cost"] > max_val:
# continue
# Create a copy of the graph that is only composed of internal hops
GG = deepcopy(self.s_graph.graph)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to follow python style guides.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I clean up the comments around here and gave it a better name too

@@ -1,6 +1,12 @@
[pycodestyle]
count = True
ignore=E121,E123,E126,E133,E226,E241,E242,E704,W503,W504,W505,E741,W605
ignore=E121,E123,E126,E133,E226,E241,E242,E704,W503,W504,W505,E741,W605,E203
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is E203?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

black (which is called by the pre-commit stack defined in the yaml file above) reformat the white space around ":" correctly but pycodetyle returns a false positive.

psf/black#279
PyCQA/pycodestyle#373

@jmmshn
Copy link
Contributor Author

jmmshn commented Jan 26, 2021

@shyuep I think this is ready for merge now.

@shyuep shyuep merged commit 0193213 into materialsvirtuallab:master Jan 26, 2021
@shyuep
Copy link
Contributor

shyuep commented Jan 26, 2021

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants